KAFKA-14478: Move LogConfig/CleanerConfig and related to storage module by ijuma · Pull Request #13049 · apache/kafka

ijuma · 2022-12-28T01:40:05Z

Additional notable changes to fix multiple dependency ordering issues:

Moved ConfigSynonym to server-common
Moved synonyms from LogConfig to ServerTopicConfigSynonyms
Removed LogConfigDef define overrides and rely on
ServerTopicConfigSynonyms instead.
Moved LogConfig.extractLogConfigMap to KafkaConfig
Consolidated relevant defaults from KafkaConfig/LogConfig in the latter
Consolidate relevant config name definitions in TopicConfig
Move ThrottledReplicaListValidator to storage

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

ijuma · 2022-12-28T04:46:32Z

storage/src/main/java/org/apache/kafka/server/log/internals/ThrottledReplicaListValidator.java

Removed the trim from here since we do it in ensureValid as well.

…MetadataLog` (#13051) `LogConfig.validateValues` may fail or incorrectly succeed if the properties don't include defaults. During the conversion of `LogConfig` to Java (#13049), it became clear that the `asInstanceOf[Long]` calls in `LogConfig.validateValues` were converting `null` to `0` when this method was invoked from `KafkaMetadataLog`. This means that it would be possible for it to validate successfully in cases where it should not. Reviewers: José Armando García Sancio <jsancio@apache.org>

Use TopicConfig whenever possible.

ijuma · 2022-12-28T20:47:31Z

core/src/main/scala/kafka/raft/KafkaMetadataLog.scala

                             maxBatchSizeInBytes: Int,
                             maxFetchSizeInBytes: Int,
-                             fileDeleteDelayMs: Int,
+                             fileDeleteDelayMs: Long,


Changed to long to match the type of the config (even if the default value happened to fit an int before).

Good catch!

ijuma · 2022-12-28T21:17:23Z

core/src/main/scala/kafka/server/KafkaConfig.scala

+      .define(LogIndexIntervalBytesProp, INT, LogConfig.DEFAULT_INDEX_INTERVAL_BYTES, atLeast(0), MEDIUM, LogIndexIntervalBytesDoc)
+      .define(LogFlushIntervalMessagesProp, LONG, LogConfig.DEFAULT_FLUSH_MESSAGES_INTERVAL, atLeast(1), HIGH, LogFlushIntervalMessagesDoc)
+      .define(LogDeleteDelayMsProp, LONG, LogConfig.DEFAULT_FILE_DELETE_DELAY_MS, atLeast(0), HIGH, LogDeleteDelayMsDoc)
+      .define(LogFlushSchedulerIntervalMsProp, LONG, LogConfig.DEFAULT_FLUSH_MS, HIGH, LogFlushSchedulerIntervalMsDoc)


It's a bit unintuitive that LogFlushSchedulerIntervalMsProp (vs LogFlushIntervalMsProp) defaults to LogConfig.DEFAULT_FLUSH_MS, but this preserves the existing approach. There is logic to set LogFlushIntervalMsProp to LogFlushSchedulerIntervalMsProp if unset.

ijuma · 2022-12-28T21:30:27Z

JDK 8 and JDK 11 builds passed, JDK 17 had one unrelated failure.

ijuma · 2022-12-28T21:44:42Z

storage/src/main/java/org/apache/kafka/server/log/internals/LogConfig.java

+        if (segmentJitterMs == 0)
+            return 0;
+        else
+            return Utils.abs(ThreadLocalRandom.current().nextInt()) % Math.min(segmentJitterMs, segmentMs);


Slight change here: we now use a ThreadLocalRandom instead of a global Random.

ijuma · 2022-12-28T21:58:35Z

storage/src/main/java/org/apache/kafka/server/log/internals/LogConfig.java

+        TopicConfig.LOCAL_LOG_RETENTION_MS_CONFIG,
+        TopicConfig.LOCAL_LOG_RETENTION_BYTES_CONFIG,
+        LEADER_REPLICATION_THROTTLED_REPLICAS_CONFIG,
+        FOLLOWER_REPLICATION_THROTTLED_REPLICAS_CONFIG


Added LEADER_REPLICATION_THROTTLED_REPLICAS_CONFIG and FOLLOWER_REPLICATION_THROTTLED_REPLICAS_CONFIG to this list since they never had server defaults.

ijuma · 2022-12-28T21:59:50Z

storage/src/main/java/org/apache/kafka/server/log/internals/LogConfig.java

+    }
+
+    // Visible for testing
+    public static class LogConfigDef extends ConfigDef {


Removed define overrides since we now rely on the statically defined ServerTopicConfigSynonyms to retrieve the server config default/synonym.

ijuma · 2023-01-02T16:52:23Z

@satishd If you have cycles, this PR is ready for review.

mimaison

Thanks. I left a few small suggestions.

mimaison · 2023-01-03T16:30:18Z

jmh-benchmarks/src/main/java/org/apache/kafka/jmh/server/CheckpointBench.java

Maybe put it with the other java.util imports below

mimaison · 2023-01-03T16:31:19Z

jmh-benchmarks/src/main/java/org/apache/kafka/jmh/partition/PartitionMakeFollowerBenchmark.java

We could inline these now

mimaison · 2023-01-03T16:40:15Z

storage/src/main/java/org/apache/kafka/server/log/internals/LogConfig.java

+        "all replicas for this topic.";
+
+    @SuppressWarnings("deprecation")
+    private static final String MESSAGE_FORMAT_VERSION_DOC = TopicConfig.MESSAGE_FORMAT_VERSION_DOC;


Do we need this constant? We already suppress deprecation in the static block below.

We do, there's no way to suppress deprecations within the static block (I made this change to get the code to compile). If you find a way to avoid this constant, please do share.

mimaison · 2023-01-03T16:59:57Z

storage/src/main/java/org/apache/kafka/server/log/internals/LogConfig.java

+    private final int maxMessageSize;
+    private final Map<?, ?> props;
+
+    public LogConfig(Map<?, ?> props) {


Considering the number of callers with new Properties() in tests I wonder if we should have a zero argument constructor too.

I would prefer to leave it as is. Generally, I don't think it's an improvement overall to add overloads that are only used in tests.

ijuma · 2023-01-03T18:14:39Z

@mimaison Thanks for the review. I addressed or replied to each comment.

satishd

Thanks @ijuma for the PR. LGTM

ijuma · 2023-01-04T10:56:43Z

I went ahead and merged this to unblock other PRs, but happy to follow up if @mimaison or @junrao have additional comments.

junrao

@ijuma : Thanks for the PR. Just a couple of minor comments.

junrao · 2023-01-05T00:16:05Z

storage/src/main/java/org/apache/kafka/server/log/internals/LogConfig.java

+        validateValues(valueMaps);
+    }
+
+    public static void main(String[] args) {


Since we moved the package of this tool, should we change the following line in build.gradle?

mainClass = 'kafka.log.LogConfig'

I observed this issue when I pulled the trunk changes in other PR and raised a fix for that in #13079

junrao · 2023-01-05T01:53:57Z

server-common/src/main/java/org/apache/kafka/server/config/ServerTopicConfigSynonyms.java

+        return Utils.mkEntry(topicConfigName, synonymsWithPrefix);
+    }
+
+    private static Entry<String, List<ConfigSynonym>> single(String topicConfigName, String brokerConfigName) {


Would differentName be more intuitive than single?

Good question. The naming scheme was trying to cover 3 different cases, each with prefix/no prefix variants:

Single mapping between topic and broker config name: single (this is like Collections.singleton in a way)

Mapping of topic to multiple broker synonyms: list

A mapping where the topic and broker config have the same name: sameName (this could also have been called identity now that I think of it).

Are you suggesting that all single* methods become differentName*? What about list*, does that become differentNameList*?

Thanks for the explanation. Got it. I am not sure that I could make the naming better. We can just keep it as it is.

…MetadataLog` (apache#13051) `LogConfig.validateValues` may fail or incorrectly succeed if the properties don't include defaults. During the conversion of `LogConfig` to Java (apache#13049), it became clear that the `asInstanceOf[Long]` calls in `LogConfig.validateValues` were converting `null` to `0` when this method was invoked from `KafkaMetadataLog`. This means that it would be possible for it to validate successfully in cases where it should not. Reviewers: José Armando García Sancio <jsancio@apache.org>

…le (apache#13049) Additional notable changes to fix multiple dependency ordering issues: * Moved `ConfigSynonym` to `server-common` * Moved synonyms from `LogConfig` to `ServerTopicConfigSynonyms ` * Removed `LogConfigDef` `define` overrides and rely on `ServerTopicConfigSynonyms` instead. * Moved `LogConfig.extractLogConfigMap` to `KafkaConfig` * Consolidated relevant defaults from `KafkaConfig`/`LogConfig` in the latter * Consolidate relevant config name definitions in `TopicConfig` * Move `ThrottledReplicaListValidator` to `storage` Reviewers: Satish Duggana <satishd@apache.org>, Mickael Maison <mickael.maison@gmail.com>

ijuma changed the title ~~KAFKA-14478: Move LogConfig and related to storage module~~ KAFKA-14478: Move LogConfig/CleanerConfig and related to storage module Dec 28, 2022

ijuma force-pushed the kafka-14478-move-log-config-storage branch from 9dbdb87 to 9532d07 Compare December 28, 2022 02:55

ijuma commented Dec 28, 2022

View reviewed changes

ijuma force-pushed the kafka-14478-move-log-config-storage branch 3 times, most recently from aaed51e to 4277ed7 Compare December 28, 2022 14:56

ijuma mentioned this pull request Dec 28, 2022

MINOR: Use LogConfig.validate instead of validateValues in KafkaMetadataLog #13051

Merged

3 tasks

ijuma added 4 commits December 28, 2022 10:37

KAFKA-14478: Reduce redundancy between LogConfig and TopicConfig

7c845c5

Use TopicConfig whenever possible.

Remove redundant defaults from LogConfig

6ae48bd

Move LogConfig to storage module

121f61c

Move CleanerConfig to storage module

f5946bd

ijuma force-pushed the kafka-14478-move-log-config-storage branch from 6bf3b84 to f5946bd Compare December 28, 2022 18:56

ijuma commented Dec 28, 2022

View reviewed changes

Remove unused variable and improve comments

823df88

ijuma marked this pull request as ready for review December 28, 2022 21:48

ijuma requested review from junrao and satishd December 28, 2022 21:55

ijuma commented Dec 28, 2022

View reviewed changes

Merge branch 'trunk' into kafka-14478-move-log-config-storage

df7abc9

mimaison reviewed Jan 3, 2023

View reviewed changes

Address review comments

d712583

satishd approved these changes Jan 4, 2023

View reviewed changes

ijuma merged commit 96d9710 into apache:trunk Jan 4, 2023

ijuma deleted the kafka-14478-move-log-config-storage branch January 4, 2023 10:42

junrao reviewed Jan 5, 2023

View reviewed changes

satishd mentioned this pull request Jan 5, 2023

MINOR Fixed doc generation for LogConfig class in genTopicConfigDocs. #13079

Merged

3 tasks

Conversation

ijuma commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Dec 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Jan 2, 2023

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Jan 3, 2023

Uh oh!

satishd left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma commented Jan 4, 2023

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

ijuma commented Dec 28, 2022 •

edited

Loading

ijuma Jan 3, 2023 •

edited

Loading